Include base_url at start of kernelspec resources path #1124

bloomsa · 2022-12-08T20:48:40Z

Resolves #1111

This ended up being a pretty minimal change. Now when returning kernelspecs from an EG, jupyter-server prepends its own base_url to the path so that any clients requesting those resources hits the proper endpoint on jupyter-server. I did not have to change anything on the gatewayClient side because JUPYTER_GATEWAY_KERNELSPECS_RESOURCE_ENDPOINT is handled already, and the addition of jupyter-server base_url in the resource path doesn't need to be handled because it's already excluded by the gateway resource handler when getting resources

Screenshots

This was taken with Jupyter-lab pointing to my local install of Jupyter-server, which was configured with `--ServerApp.base_url=base`. I also tested the case where no `base_url` was present.

Tests

I didn't modify any test cases, but it doesn't look like any failed. I'm guessing that's because the tests don't have a resources block in the kernelspec. Would you like me to add that as part of this PR?

codecov · 2022-12-08T20:52:57Z

Codecov Report

Base: 80.04% // Head: 79.99% // Decreases project coverage by -0.05% ⚠️

Coverage data is based on head (2c8cb2e) compared to base (a55bc58).
Patch coverage: 92.30% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1124      +/-   ##
==========================================
- Coverage   80.04%   79.99%   -0.06%     
==========================================
  Files          68       68              
  Lines        8040     8053      +13     
  Branches     1588     1591       +3     
==========================================
+ Hits         6436     6442       +6     
- Misses       1182     1186       +4     
- Partials      422      425       +3

Impacted Files	Coverage Δ
jupyter_server/gateway/managers.py	`82.97% <92.30%> (+0.29%)`	⬆️
...ter_server/services/kernels/connection/channels.py	`58.49% <0.00%> (-1.11%)`	⬇️
jupyter_server/serverapp.py	`79.82% <0.00%> (-0.09%)`	⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

kevin-bates · 2022-12-08T23:24:05Z

hi @bloomsa - thanks for the PR! I've gone ahead and queued myself as a reviewer.

At quick glance, I guess I was hoping we could contain these "gateway-related" changes in the gateway directory since the if block you've modified implies the kernelspecs are from a gateway. I'd rather us not "leak" gateway-specific logic outside jupyter_server/gateway. As I noted earlier, the location for these changes would be in GatewayKernelSpecManager and I believe you can access the local base_url value from its self.parent.base_url attribute.

Also, I don't see logic to handle the replacement of a potential base_url that is configured on the Gateway server and included in its kernelspecs responses. I think we need to handle that scenario as well. Thank you.

bloomsa · 2022-12-09T15:45:37Z

At quick glance, I guess I was hoping we could contain these "gateway-related" changes in the gateway directory since the if block you've modified implies the kernelspecs are from a gateway. I'd rather us not "leak" gateway-specific logic outside jupyter_server/gateway. As I noted earlier, the location for these changes would be in GatewayKernelSpecManager and I believe you can access the local base_url value from its self.parent.base_url attribute.

I totally agree on this point. I initially tried to keep the changes limited to that class but I couldn't figure out how to get the server's base_url from there. I'll go ahead and make that change asap.

Also, I don't see logic to handle the replacement of a potential base_url that is configured on the Gateway server and included in its kernelspecs responses. I think we need to handle that scenario as well. Thank you

With regards to this, I believe that base url would always be part of the JUPYTER_GATEWAY_URL that is configured when the Jupyter-server starts. If it wasn't there or in JUPYTER_GATEWAY_KERNELSPECS_RESOURCE_ENDPOINT, we would have to persist it between requests somehow.
To that point though, these changes to jupyter-server would trickle down to EG right? (I think that's part of what you're implying) so when kernelspecs are requested of EG, they will include the base_url for it, if configured. I will have to strip that out of the kernelspecs response from EG, probably in the same method where I add the jupyter-server's base_url.

Thank you for the quick review and pointing me in the right direction. It's much appreciated :) @kevin-bates

kevin-bates · 2022-12-09T20:01:57Z

With regards to this, I believe that base url would always be part of the JUPYTER_GATEWAY_URL that is configured when the Jupyter-server starts.

Correct. Using the example below in which my EG was launched using --EnterpriseGatewayApp.base_url=foo/fighters, I would reference it when starting jupyter server using --gateway-url=http://localhost:8889/foo/fighters.

To that point though, these changes to jupyter-server would trickle down to EG right? (I think that's part of what you're implying) so when kernelspecs are requested of EG, they will include the base_url for it, if configured. I will have to strip that out of the kernelspecs response from EG, probably in the same method where I add the jupyter-server's base_url.

Yes. So with foo/fighters as the base url, the kernelspecs returned to jupyter server from EG would have resource entries like this (using the R kernel on K8s as an example):

      "resources": {
        "kernel.js": "foo/fighters/kernelspecs/k8s_r/kernel.js",
        "logo-64x64": "foo/fighters/kernelspecs/k8s_r/logo-64x64.png"
      }

where, if starting jupyter server with --ServerApp.base_url=bar, would require replacement of foo/fighters/kernelspecs with bar/kernelspecs.

Regarding this clause: these changes to jupyter-server would trickle down to EG. Just to clarify, no changes to EG should be required. I suspect you know that and was just stating that the EG resources may also be prefixed with EG's relative base-url.

bloomsa · 2022-12-10T16:44:26Z

This is ready for re-review. I tried to avoid the nested for loop in the helper method but I couldn't figure out a better way of doing it since the json response object from a Gateway has a nested structure. The code now handle the following situations:

jupyter_server has a base_url, the gateway does not - jupyter_server base_url is prepended to the resources paths returned by Gateway
jupyter_server does not have a base_url, the gateway also does not - nothing happens in the helper method
jupyter_server has a base_url, the gateway does as well - jupyter_server replaces the gateway's base_url with its own. this allows the client to reach the right endpoint on jupyter_server. The gateway base_url gets re-added to the url path when the server builds the resource request for the gateway
jupyter_server does not have a base_url, the gateway does - jupyter_server strips the gateway base_url from the resources path, this allows the client to reach the right endpoint on jupyter_server. The gateway base_url gets re-added to the url path when the server builds the resource request for the gateway

screenshots

this is an example of situation 3 listed above

kevin-bates

Hi @bloomsa - this is looking good and works well. I just had some comments around the logging and perhaps improving on the test at some point. Thanks.

kevin-bates · 2022-12-11T16:58:02Z

jupyter_server/gateway/managers.py

+                kernel_specs["kernelspecs"][kernel_name]["resources"][
+                    resource_name
+                ] = url_path_join(self.parent.base_url, "/kernelspecs", split_eg_base_url[1])
+                self.log.debug(


Could we please make this logging conditional on there being a replacement? Otherwise, this will log for 80% of cases when base_url values are not in play.

good call. I'll add a conditional check.

jupyter_server/gateway/managers.py

tests/test_gateway.py

kevin-bates

Thanks @bloomsa!

welcome · 2022-12-13T02:27:17Z

Congrats on your first merged pull request in this project! 🎉

Thank you for contributing, we are very proud of you! ❤️

lresende · 2022-12-18T05:46:56Z

@meeseeksdev please backport to 1.x

…pec resources path

…pec resources path) (#1138) Co-authored-by: Sam Bloomquist <[email protected]>

Sam Bloomquist and others added 2 commits December 8, 2022 10:11

formatting fix

c2f66d5

Merge branch 'jupyter-server:main' into main

06f08ec

Merge branch 'main' into main

7221945

kevin-bates self-requested a review December 8, 2022 22:51

kevin-bates added the bug label Dec 8, 2022

Sam Bloomquist added 2 commits December 10, 2022 10:29

Moved method into gateway classes

c48da52

Remove moved method

eba2ba3

kevin-bates reviewed Dec 11, 2022

View reviewed changes

conditional on debug log, validate base_url in testcase

2c8cb2e

kevin-bates reviewed Dec 13, 2022

View reviewed changes

kevin-bates mentioned this pull request Dec 13, 2022

Look at handling leading slash - base_url on kernelspec resources jupyter-server/enterprise_gateway#1219

Open

blink1073 merged commit dd04a01 into jupyter-server:main Dec 13, 2022

meeseeksmachine mentioned this pull request Dec 18, 2022

Backport PR #1124 on branch 1.x (Include base_url at start of kernelspec resources path) #1138

Merged

meeseeksmachine pushed a commit to meeseeksmachine/jupyter_server that referenced this pull request Dec 18, 2022

Backport PR jupyter-server#1124: Include base_url at start of kernels…

a2d3abc

…pec resources path

blink1073 pushed a commit that referenced this pull request Dec 20, 2022

Backport PR #1124 on branch 1.x (Include base_url at start of kernels…

0a02e42

…pec resources path) (#1138) Co-authored-by: Sam Bloomquist <[email protected]>

kevin-bates mentioned this pull request Jan 18, 2023

Don't assume that resources entries are relative #1182

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Include base_url at start of kernelspec resources path #1124

Include base_url at start of kernelspec resources path #1124

bloomsa commented Dec 8, 2022 •

edited

Loading

codecov bot commented Dec 8, 2022 •

edited

Loading

kevin-bates commented Dec 8, 2022

bloomsa commented Dec 9, 2022 •

edited

Loading

kevin-bates commented Dec 9, 2022

bloomsa commented Dec 10, 2022

kevin-bates left a comment

kevin-bates Dec 11, 2022

bloomsa Dec 12, 2022

kevin-bates left a comment

welcome bot commented Dec 13, 2022

lresende commented Dec 18, 2022

Include base_url at start of kernelspec resources path #1124

Include base_url at start of kernelspec resources path #1124

Conversation

bloomsa commented Dec 8, 2022 • edited Loading

Screenshots

Tests

codecov bot commented Dec 8, 2022 • edited Loading

Codecov Report

kevin-bates commented Dec 8, 2022

bloomsa commented Dec 9, 2022 • edited Loading

kevin-bates commented Dec 9, 2022

bloomsa commented Dec 10, 2022

screenshots

kevin-bates left a comment

Choose a reason for hiding this comment

kevin-bates Dec 11, 2022

Choose a reason for hiding this comment

bloomsa Dec 12, 2022

Choose a reason for hiding this comment

kevin-bates left a comment

Choose a reason for hiding this comment

welcome bot commented Dec 13, 2022

lresende commented Dec 18, 2022

bloomsa commented Dec 8, 2022 •

edited

Loading

codecov bot commented Dec 8, 2022 •

edited

Loading

bloomsa commented Dec 9, 2022 •

edited

Loading